Skip to content

Integrate yellowstone-vixen-parser with codama #348

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 29 commits into from
Jan 28, 2025
Merged

Integrate yellowstone-vixen-parser with codama #348

merged 29 commits into from
Jan 28, 2025

Conversation

@Nagaprasadvr
Copy link
Contributor

@Nagaprasadvr Nagaprasadvr commented Dec 5, 2024 •
edited
Loading

Goal

yellostone-vixen-parser crate helps with parsing accounts , instructions
We can automate the process of generating these rust acc and ixs parsers using codama idl so anyone can add this as a plugin and parse their program state and ixs effectively in rust

  • Add accounts_parser template
  • Add instructions_parser template
  • Add logic to parse codama idl and generate rust code for these parsers

@changeset-bot
Copy link

changeset-bot bot commented Dec 5, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 8604bcf

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@codama/renderers-rust Patch
@codama/renderers-vixen-parser Patch
@codama/renderers Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Nagaprasadvr Nagaprasadvr marked this pull request as draft December 5, 2024 07:04
@Nagaprasadvr Nagaprasadvr changed the title Integrate vixen parser with codama Integrate yellostone-vixen-parser with codama Dec 5, 2024
@Nagaprasadvr Nagaprasadvr marked this pull request as ready for review December 16, 2024 10:41
@Nagaprasadvr Nagaprasadvr force-pushed the vix-33-integrate-vixen-parser-with-codama branch from 07e0b77 to 8574dc8 Compare December 17, 2024 06:35
lorisleiva
lorisleiva reviewed Dec 19, 2024
View reviewed changes
lorisleiva
lorisleiva reviewed Dec 19, 2024
View reviewed changes
febo
febo reviewed Dec 20, 2024
View reviewed changes
febo
febo reviewed Dec 20, 2024
View reviewed changes
@Nagaprasadvr Nagaprasadvr force-pushed the vix-33-integrate-vixen-parser-with-codama branch from 63485d6 to c2c98a8 Compare December 23, 2024 07:25
@Nagaprasadvr Nagaprasadvr force-pushed the vix-33-integrate-vixen-parser-with-codama branch 3 times, most recently from b6f82dd to 7584669 Compare December 26, 2024 07:38
@Nagaprasadvr Nagaprasadvr changed the title Integrate yellostone-vixen-parser with codama Integrate yellowstone-vixen-parser with codama Jan 10, 2025
@kespinola kespinola mentioned this pull request Jan 21, 2025
Closed
kespinola
kespinola previously approved these changes Jan 21, 2025
View reviewed changes
lorisleiva
lorisleiva requested changes Jan 23, 2025
View reviewed changes
Copy link
Member

@lorisleiva lorisleiva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made a few comments and we need to make sure CI passes before this can be merged but looks good overall! Also I think this needs a quick rebase from the main branch.

I don't understand the entire logic around the rendered parsers but one thing I noticed is it seems to discriminate all accounts by length instead of using the information provided by the DiscriminatorNodes in the standard. Not sure if that's by design at the moment but thought it needed to be flagged.

@Nagaprasadvr Nagaprasadvr force-pushed the vix-33-integrate-vixen-parser-with-codama branch from 1bfe88c to f576eaf Compare January 23, 2025 13:27
@Nagaprasadvr Nagaprasadvr reopened this Jan 23, 2025
@Nagaprasadvr Nagaprasadvr force-pushed the vix-33-integrate-vixen-parser-with-codama branch from a79f7bc to 7252134 Compare January 23, 2025 13:39
lorisleiva
lorisleiva requested changes Jan 23, 2025
View reviewed changes
Copy link
Member

@lorisleiva lorisleiva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI not passing sorry.

@Nagaprasadvr
Copy link
Contributor Author

Nagaprasadvr commented Jan 23, 2025

CI not passing sorry.

Wil fix them

lorisleiva
lorisleiva requested changes Jan 24, 2025
View reviewed changes
Copy link
Member

@lorisleiva lorisleiva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just need to commit the generated changes in e2e tests now.

@Nagaprasadvr
Copy link
Contributor Author

Nagaprasadvr commented Jan 24, 2025

Just need to commit the generated changes in e2e tests now.

I don't see any local changes on my system

@lorisleiva
Copy link
Member

lorisleiva commented Jan 24, 2025

@Nagaprasadvr even after running the tests?

@Nagaprasadvr
Copy link
Contributor Author

Nagaprasadvr commented Jan 24, 2025 •
edited
Loading

@Nagaprasadvr even after running the tests?

Yup, if u rerun the tests it might work ?

@lorisleiva
Copy link
Member

lorisleiva commented Jan 27, 2025

Strange, I also don't see any changes locally so it must be a CI thing. I'm trying to push commits to this branch to debug but it looks like I'm not allowed. Is there anything you can toggle to allow me to push to this PR?

@lorisleiva
Copy link
Member

lorisleiva commented Jan 27, 2025

We also need a changeset so we can trigger a publish of the new library on merge.

@Nagaprasadvr
Copy link
Contributor Author

Nagaprasadvr commented Jan 27, 2025

Strange, I also don't see any changes locally so it must be a CI thing. I'm trying to push commits to this branch to debug but it looks like I'm not allowed. Is there anything you can toggle to allow me to push to this PR?

Will give u access

@Nagaprasadvr
Copy link
Contributor Author

Nagaprasadvr commented Jan 27, 2025

We also need a changeset so we can trigger a publish of the new library on merge.

Wil add

@Nagaprasadvr
Copy link
Contributor Author

Nagaprasadvr commented Jan 28, 2025

where do we add the changeset?

@lorisleiva
Copy link
Member

lorisleiva commented Jan 28, 2025

I've just done it but, for next time, you just run changeset in the root directory locally and follow the prompts.

lorisleiva
lorisleiva approved these changes Jan 28, 2025
View reviewed changes
Copy link
Member

@lorisleiva lorisleiva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we're good to merge! 👌

@lorisleiva lorisleiva merged commit f70b407 into codama-idl:main Jan 28, 2025
4 checks passed
@github-actions github-actions bot mentioned this pull request Jan 28, 2025
Merged
@slimeonmyhead slimeonmyhead mentioned this pull request Feb 11, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lorisleiva lorisleiva lorisleiva approved these changes

@febo febo Awaiting requested review from febo

@kespinola kespinola Awaiting requested review from kespinola

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Nagaprasadvr @lorisleiva @febo @kespinola